Add workflow to build package and run tests #8
Conversation
…github-workflow-maven
|
Caution Review failedThe pull request is closed. WalkthroughA new Continuous Integration (CI) workflow for Java projects using Maven automates builds and tests on changes to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/main/java/de/tum/cit/ase/ares/api/ast/model/RecursionCheck.java (1 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/AstAssertionTest.java (1 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/testuser/PathAccessUser.java (1 hunks)
Files skipped from review due to trivial changes (2)
- src/main/java/de/tum/cit/ase/ares/api/ast/model/RecursionCheck.java
- src/test/java/de/tum/cit/ase/ares/integration/AstAssertionTest.java
Additional comments not posted (1)
src/test/java/de/tum/cit/ase/ares/integration/testuser/PathAccessUser.java (1)
Line range hint
69-69:
Verify the intentional removal of the security policy.The
@Policyannotation, which specified a security policy file and a path constraint, has been removed from theweAccessPathmethod. Ensure that this change is intentional and does not compromise the security context of the test.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/maven.yml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/maven.yml
There was a problem hiding this comment.
Actionable comments posted: 9
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/CustomClassResolver.java (2 hunks)
- src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/TransitivelyAccessesMethodsCondition.java (3 hunks)
- src/main/resources/archunit.properties (1 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/PathAccessTest.java (1 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/testuser/PathAccessUser.java (1 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/pathaccess/PathAccessPenguin.java (1 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java (1 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/thirdpartypackage/ThirdPartyPackagePenguin.java (1 hunks)
Files skipped from review due to trivial changes (2)
- src/main/resources/archunit.properties
- src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/pathaccess/PathAccessPenguin.java
Files skipped from review as they are similar to previous changes (1)
- src/test/java/de/tum/cit/ase/ares/integration/testuser/PathAccessUser.java
Additional comments not posted (19)
src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/thirdpartypackage/ThirdPartyPackagePenguin.java (1)
15-17: Verify the existence of the file during tests.Ensure that the file
fileUsingFilesClass.txtexists in the expected location during tests to avoidFileNotFoundException.src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/CustomClassResolver.java (1)
Line range hint
1-24:
Verify the impact of the removed class resolver setting.The removal of the line
ArchConfiguration.get().setClassResolver(ClassResolverFromClasspath.class);might affect the class resolution mechanism. Ensure that the class resolution process works as expected without this setting.src/test/java/de/tum/cit/ase/ares/integration/PathAccessTest.java (1)
103-106: Verify the exception handling during the test.Ensure that the
SecurityExceptionis correctly handled and reported during the test.Verification successful
Verify the exact implementation of the
test_accessFileSystemmethod.Ensure that the method correctly handles and asserts the
SecurityException.
Exception handling in
test_accessFileSystemis correctly implemented.The method correctly asserts that a
SecurityExceptionis thrown when accessing the file system.
src/test/java/de/tum/cit/ase/ares/integration/PathAccessTest.java: test_accessFileSystemScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the exception handling during the test. # Test: Search for the `SecurityException` handling. Expect: Proper handling of the exception. rg --type java 'SecurityException'Length of output: 19328
Script:
#!/bin/bash # Description: Retrieve the implementation of the `test_accessFileSystem` method to verify exception handling. # Search for the `test_accessFileSystem` method in the PathAccessTest.java file. rg --type java '@TestTest' -A 5Length of output: 206213
src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/TransitivelyAccessesMethodsCondition.java (5)
15-15: Imports look good.The addition of
java.util.*is necessary for the new functionality.
30-38: Addition ofbannedClassesis appropriate.This set refines the criteria for determining access violations by excluding certain fundamental Java classes and packages from consideration.
50-54: Addition ofresolvedClassesis appropriate.This map optimizes performance by caching resolved classes and preventing redundant resolution attempts.
61-61: Initialization ofresolvedClassesis appropriate.Initializing the map as a
HashMapensures it is ready for use.
166-178: Changes togetDirectAccessTargetsOutsideOfAnalyzedClassesare appropriate.The method now checks if the accessed class is part of
bannedClassesand usesresolvedClassesto optimize class resolution, enhancing efficiency and ensuring adherence to the new access rules.src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java (11)
44-50: MethodaccessFileSystemViaFileInputStreamlooks good.The method is straightforward and uses a valid file path.
55-62: MethodaccessFileSystemViaFileOutputStreamlooks good.The method is straightforward and uses a valid file path.
67-79: MethodaccessFileSystemViaFileReaderlooks good.The method is straightforward and uses a valid file path.
84-93: MethodaccessFileSystemViaFileWriterlooks good.The method is straightforward and uses a valid file path.
98-113: MethodaccessFileSystemViaRandomAccessFilelooks good.The method is straightforward and uses a valid file path.
118-143: MethodaccessFileSystemViaFilelooks good.The method is straightforward and uses a valid file path.
234-250: MethodaccessFileSystemViaFileSystemlooks good.The method is straightforward and does not use
nullfor file paths.
272-295: MethodaccessFileSystemViaPathlooks good.The method is straightforward and uses valid file paths.
300-309: MethodaccessFileSystemViaPathslooks good.The method is straightforward and uses valid file paths.
314-325: MethodaccessFileSystemViaWatchServicelooks good.The method is straightforward and does not use
nullfor file paths.
360-369: MethodaccessFileSystemViaFileHandlerlooks good.The method is straightforward and uses valid file paths.
...cit/ase/ares/api/architecturetest/java/postcompile/TransitivelyAccessesMethodsCondition.java
Outdated
Show resolved
Hide resolved
...t/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...t/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...t/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...t/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...t/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...t/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...t/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...t/java/de/tum/cit/ase/ares/integration/testuser/subject/student/FileSystemAccessPenguin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/TransitivelyAccessesMethodsCondition.java (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/TransitivelyAccessesMethodsCondition.java
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/CustomClassResolver.java (1 hunks)
- src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/JavaArchitectureTestCaseCollection.java (1 hunks)
- src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/TransitivelyAccessesMethodsCondition.java (4 hunks)
- src/main/resources/archunit.properties (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/resources/archunit.properties
Additional comments not posted (8)
src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/CustomClassResolver.java (3)
4-6: LGTM!The new import statements for
JavaClassesandImportOptionare necessary and appropriate for the new implementation.
19-26: LGTM! Consider improving readability.The declaration and initialization of
allClassesare correct. For better readability, consider chaining the import options more clearly.- allClasses = new ClassFileImporter() - .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) - .withImportOption(location -> location.toString().contains("jrt:/")) - .importClasspath(); + allClasses = new ClassFileImporter() + .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) + .withImportOption(location -> location.toString().contains("jrt:/")) + .importClasspath();
34-39: LGTM!The
tryResolvemethod correctly usesallClassesto resolve the class and handles exceptions appropriately.src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/JavaArchitectureTestCaseCollection.java (1)
50-50: Verify the removal of banned packages.The removal of
"sun.security"and"java.awt.desktop"packages likely indicates a change in security or functionality requirements. Ensure that this change is intentional and does not introduce security risks.Verification successful
Verified the removal of banned packages.
The removal of
sun.securityandjava.awt.desktoppackages from the banned list is confirmed to be safe as there are no occurrences of these packages in the codebase or configuration/documentation files.
- No occurrences of
sun.securityorjava.awt.desktopfound in the codebase.- No mentions of
sun.securityorjava.awt.desktopfound in configuration or documentation files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of banned packages. # Test: Search for the usage of the removed packages. Expect: No occurrences of the removed packages. rg --type java 'sun.security|java.awt.desktop'Length of output: 48
Script:
#!/bin/bash # Description: Verify the removal of banned packages in configuration or documentation files. # Test: Search for mentions of the removed packages in configuration or documentation files. rg 'sun.security|java.awt.desktop' --glob '*.{yml,yaml,json,md,xml,properties}'Length of output: 81
src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/TransitivelyAccessesMethodsCondition.java (4)
30-39: LGTM!The
bannedClassesset includes fundamental Java classes and packages, which streamlines access violation checks.
52-55: LGTM!The declaration and initialization of
customClassResolverare correct and use theCustomClassResolverclass appropriately.Also applies to: 77-77
166-186: LGTM!The method
getDirectAccessTargetsOutsideOfAnalyzedClassescorrectly checks if the accessed class is part ofbannedClassesand resolves target owners usingcustomClassResolver.
199-201: LGTM!The method
resolveTargetOwnercorrectly usescustomClassResolverto resolve class names, improving the efficiency of access checks.
…ompile tests etc.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/maven.yml (1 hunks)
- src/main/resources/logback.xml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/maven.yml
Additional comments not posted (1)
src/main/resources/logback.xml (1)
15-15: LGTM! Ensure the new logger is used as intended.The addition of the logger for
com.tngtech.archunitwith a logging level set toINFOis a good enhancement for better logging granularity.Verify that the new logger is effectively utilized in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/CustomClassResolver.java (1 hunks)
- src/main/resources/archunit.properties (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/resources/archunit.properties
Additional comments not posted (4)
src/main/java/de/tum/cit/ase/ares/api/architecturetest/java/postcompile/CustomClassResolver.java (4)
4-4: LGTM! The import statement is necessary.The import statement for
JavaClassesis required for the newallClassesvariable.
17-17: LGTM! The variable declaration is necessary.The new instance variable
allClassesis required to hold all imported classes.
19-25: Consider addressing the memory efficiency concern.The constructor imports all classes, which might not be memory efficient. The TODO comment suggests improving this.
Ensure that the memory efficiency concern is addressed in future updates.
33-38: LGTM! The method change is appropriate.The
tryResolvemethod now directly accessesallClassesto resolve classes, handling cases where the class may not exist by returning an emptyOptional.
Checklist
General
Ares
Motivation and Context
This PR solves a bug within the ArchRules that was not detected due to abstract and interface classes. It changes the ClassLoader to import all related classes to be able to retrieve the necessary metadata for the architectural tests. It also introduces a CI workflow to be able to see if some things broke the tests.
Description
Introduced a new CI workflow for building and testing Ares2.0. Modified formatVertexInfoWithinCycle to use File.separator for improved OS compatibility. Removed explicit class resolver setting, reverting to default resolution. Enhanced method access management with bannedClasses.
Bug Fixes
Documentation
Review Progress
Code Review
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores
Summary by CodeRabbit
New Features
Bug Fixes
Documentation